feat(storage): separate read and hedging thread pools - #16389
Conversation
product-auto-label
Bot
added
the
api: storage
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the storage client's thread pool management by introducing a dedicated ThreadPool for primary read attempts, distinct from the HedgingThreadPool used for speculative hedges. It adds configuration options for thread pool sizes and updates the connection implementation, read source, and tests accordingly. The review feedback identifies a namespace compilation error in client.cc, requests the use of explicit types instead of auto for primitives in connection_impl.cc to comply with the style guide, and suggests caching thread pool size calculations in hedging_thread_pool.h for better performance.
c4ac18d to
40cf649
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #16389 +/- ##
==========================================
+ Coverage 92.24% 92.25% +0.01%
==========================================
Files 2246 2246
Lines 212296 212567 +271
==========================================
+ Hits 195825 196104 +279
+ Misses 16471 16463 -8 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
928f74a to
edddb77
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a dedicated ThreadPool class to isolate primary read attempts from speculative hedge attempts, which previously shared a single HedgingThreadPool. The HedgingThreadPool has been refactored to delegate task execution to an internal ThreadPool while retaining its throttling and rate-limiting capabilities. Additionally, new configuration options (ReadThreadPoolSizeOption and HedgingThreadPoolSizeOption) and their corresponding default sizing logic have been added to allow fine-grained control over pool sizes. Unit tests have been expanded to verify thread pool isolation, saturation behaviors, and safe destruction. There are no review comments to address, and the changes conform to the repository's style guidelines.
|
/gcbrun |
edddb77 to
d8d4184
Compare
|
/gcbrun |
65f06fb to
9160561
Compare
- Extract lazy, dynamically scaling ThreadPool primitive from HedgingThreadPool. - Separate StorageConnectionImpl thread pool into a dedicated ReadThreadPool (for primary stream opens) and a HedgingThreadPool (for speculative secondary hedges). - Add ReadThreadPoolSizeOption and HedgingThreadPoolSizeOption with auto-scaling defaults to prevent read bottlenecking under high concurrency. - Extract DefaultReadThreadPoolSize() and DefaultHedgingThreadPoolSize() helpers to share sizing logic between DefaultOptions() and connection initialization. - Enqueue primary read attempt to ReadThreadPool and speculative hedge attempts to HedgingThreadPool, ensuring complete fault and stall isolation. - Clamp ThreadPool capacity to at least 1 to prevent deadlock on zero sizing. - Add unit tests verifying thread pool execution, default sizes, zero-size handling, lazy spawning, and pool isolation under saturation.
c5f78ad to
2bf8045
Compare
87d6180 to
66f7d18
Compare
|
/gcbrun |
1 similar comment
|
/gcbrun |
3 participants
This PR separates the thread pools used for primary reads and speculative hedged reads in Cloud Storage read hedging.
Previously, both primary reads and speculative hedges shared a single thread pool. This introduced two architectural issues:
attempts were blocked from executing, defeating the primary purpose of hedging (tail latency mitigation).
whereas speculative hedges are gated by rate limits and concurrency controls (typically bounded by MaxConcurrentHedgesOption or 2 × hardware concurrency).
This change introduces a general-purpose, lazily-scaling hedging_thread_pool.h:49 and composes it within hedging_thread_pool.h:145, isolating primary read execution from speculative hedges.
Key Changes
1. Dedicated ThreadPool and Embedded HedgingThreadPool
hedging_thread_pool.h:49:
• Dynamically scales workers on demand up to max_threads.
• Workers wait on a condition variable when idle and exit gracefully on shutdown.
• Automatically clamps max_threads to ≥1 to prevent deadlock/infinite hang if configured with 0.
• Supports self-destruction from within a worker thread (safely detaches rather than joining itself).
hedging_thread_pool.h:145:
• Embeds hedging_thread_pool.h:235 by value as its execution backend (declared last to guarantee worker joining before state teardown).
• Enforces the token-bucket rate limiter (ReadHedgeRateLimitOption) and maximum concurrent hedge ceiling (MaxConcurrentHedgesOption).
2. Dual Pool Configuration & Sizing Options
• Added options.h:122: Defaults to DefaultReadThreadPoolSize() (max (64,4 × cores)).
• Added options.h:135: Defaults to DefaultHedgingThreadPoolSize() (MaxConcurrentHedgesOption if set, else max (16,2 × cores)).
• Centralized sizing defaults in DefaultReadThreadPoolSize() and DefaultHedgingThreadPoolSize() so client.cc:604 and connection_impl.cc:165 remain consistent.
3. Isolation in HedgedObjectReadSource
• Updated hedged_object_read_source.cc:90 to accept separate read_pool_ and hedge_pool_.
• Primary attempt opens are scheduled onto read_pool_.
• Speculative hedged attempts are scheduled onto hedge_pool_.